-
-
Notifications
You must be signed in to change notification settings - Fork 17.6k
docopts: Expose docopts.sh as well as docopts itself #408168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
I believe this change is suitable for backports, if you agree and you're a maintainer I'd appreciate if you could add the backport labels :) |
Not sure if NixOS/nixpkgs#408168 will get reviewed.
|
Er, hold up a minute... {
description = "A very basic flake";
inputs = {
nixpkgs.url = "github:bjackman/nixpkgs?ref=docopts-sh";
};
outputs = { self, nixpkgs }: {
packages.x86_64-linux.default =
let pkgs = import nixpkgs { system = "x86_64-linux"; };
in pkgs.writeShellApplication {
name = "hello";
runtimeInputs = [ pkgs.docopts ];
text = "source docopts.sh";
};
};
}Seems I did not test this properly before. |
|
Ah right yeah this is just a foible of So to take advantage of this you do need to use some different logic for wrapping up the script, or you need to extend |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5676 |
c1dda74 to
865991b
Compare
Docopts also includes a shell wrapper. This can be sourced, which provides a slightly more ergonomic interface. More importantly though, this is what the examples in the docopts repository use, so it's much more accessible. Signed-off-by: Brendan Jackman <bhenryj0117@gmail.com>
|
Oops, fixed the formatting. |
|
lgtm. Although it seems, that just adding another patch to the |
| { print }' docopts.sh | ||
| # Now, since this will be sourced by the user and we don't want to clobber | ||
| # PATH in their scripts, add a line at the end to restore the old PATH. | ||
| echo 'PATH="$__NIX_OLD_PPATH"' >> docopts.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... it is a bit cursed. Does this really work? docopts.sh exports several functions like docopt_get_raw_value which calls awk. Resetting PATH at the end of the script probably breaks that, right?
I'm not sure I have a clean solution. I'd say maybe just append to the PATH only, and don't restore at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes thank you you are right. I only thought about the wrapper mode but this will break the library mode.
This kinda just seems like an incompatiility between the "yolo everything is global" model of Bash and the principles of Nix.
I don't really like clobbering people's path when they source the script... In fact, since I first wrote this PR, I decided I also don't really like docopts all that much. So much so that maybe I'd rather not package docopts.sh at all, than package it with the "clobbers $PATH" issue... I will ponder it and get back to you!
(Google has an internal Bash library for arg parsing in the style of Abseil/Go that I like much more. It's much much simpler. Let me know if you know of anything like this for Bash! Otherwise maybe I'll try to make an open source version...)
Docopts also includes a shell wrapper. This can be sourced, which provides a slightly more ergonomic interface. More importantly though, this is what the examples in the docopts repository use, so it's much more accessible.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truewriteShellApplicationscript to sourcedocopts.shnix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.